Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comment TraceLevelInfoFilter as it breaks OTel compatibility #21

Merged
merged 1 commit into from
May 25, 2024

Conversation

vikmik
Copy link
Contributor

@vikmik vikmik commented May 23, 2024

TraceLevelInfoFilter isn't used - its only use was in

// otelgrpc.WithInterceptorFilter(TraceLevelInfoFilter),
but is currently commented.

This PR unblocks consumers of gubernator who also use the latest OTel libraries (version 0.52), as the type otelgrpc.Filter changed in open-telemetry/opentelemetry-go-contrib#5196 ). See change here. The code I commented is now invalid with latest versions of OTel, which prevents consumers of OTel libraries to upgrade them if they also use gubernator. Building gubernator with the latest OTel libraries fails with:

# github.com/gubernator-io/gubernator/v2
../../../../pkg/mod/github.com/gubernator-io/gubernator/v2@v2.7.4/config.go:739:44: cannot convert func(info *otelgrpc.InterceptorInfo) bool {…} (value of type func(info *otelgrpc.InterceptorInfo) bool) to type otelgrpc.Filter

Allegedly this could now be fixed with the merge of the above PR (and the code in daemon.go could be uncommented), but it requires a larger change.

I am opening this PR to unblock users of OTel and gubernator as it is the most pressing issue.


Commenting feels a bit weird (I did it because of the commented code in daemon.go), let me know if you want to address this differently.

@vikmik vikmik requested a review from Baliedge as a code owner May 23, 2024 10:36
@vikmik
Copy link
Contributor Author

vikmik commented May 24, 2024

Also cc @thrawn01 just in case :)

@thrawn01
Copy link
Collaborator

There have been so many golang mod compatibility errors with otel, over the past few months. I'm beginning to wonder if it's worth the hassle.

The maintainers of different otel libraries don't appear to be managing releases properly. 😞

@thrawn01
Copy link
Collaborator

For my Ref: open-telemetry/opentelemetry-go-contrib#4575 no deprecation notice, with no suggestions of an alternative. 👎

Copy link
Collaborator

@thrawn01 thrawn01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can delete this code if you wish, but I'll accept the PR as is. If I don't hear back from you in the next 12 hrs, I'll merge.

Thank you for making this PR!

@thrawn01 thrawn01 merged commit 1cafc99 into gubernator-io:master May 25, 2024
4 checks passed
@vikmik
Copy link
Contributor Author

vikmik commented May 25, 2024

Just seeing this. Thank you! Yes it seems that having an OTel dependency can make things difficult. I made this PR recently too. Thanks for merging this PR

@thrawn01 thrawn01 mentioned this pull request Jun 12, 2024
@pbennett
Copy link

pbennett commented Sep 8, 2024

There have been so many golang mod compatibility errors with otel, over the past few months. I'm beginning to wonder if it's worth the hassle.

The maintainers of different otel libraries don't appear to be managing releases properly. 😞

I'm inclined to agree. I use some other packages that recently switched (somewhat forced) to otel as well and it seems like it might becoming a bit of a nightmare dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants